codeframe/core/installer.py (3)

38-51: Consider logging when defaulting to linux for unknown platforms.

When platform.system() returns an unexpected value (e.g., "FreeBSD"), silently defaulting to "linux" may cause confusing failures. A debug log would aid troubleshooting.

🔧 Suggested improvement

     system = platform.system()
     if system == "Linux":
         return "linux"
     elif system == "Darwin":
         return "darwin"
     elif system == "Windows":
         return "windows"
+    logger.debug(f"Unknown platform '{system}', defaulting to linux")
     return "linux"  # Default to linux

---

265-272: Missing TimeoutExpired handling unlike PipInstaller.

PipInstaller explicitly catches subprocess.TimeoutExpired (lines 172-178), but NpmInstaller catches only generic Exception. This loses timeout-specific context in error messages. The same inconsistency exists in CargoInstaller.

♻️ Add explicit timeout handling for consistency

+        except subprocess.TimeoutExpired:
+            return InstallResult(
+                tool_name=tool_name,
+                status=InstallStatus.FAILED,
+                message=f"Installation of {tool_name} timed out",
+                command_used=command,
+            )
         except Exception as e:
             return InstallResult(

---

537-543: Silent JSONDecodeError handling loses diagnostic context.

When the history file is corrupted, silently falling back to an empty dict makes the corruption invisible to debugging. Consider logging a warning.

🔧 Add warning log for corrupted history file

         if self.history_file.exists():
             try:
                 with open(self.history_file) as f:
                     history = json.load(f)
             except json.JSONDecodeError:
-                pass
+                logger.warning(f"Corrupted history file at {self.history_file}, starting fresh")

codeframe/core/environment.py (1)

638-652: _detect_conflicts is a stub with no implementation.

The method always returns an empty list. The comment mentions it "could be expanded," but the PR summary advertises conflict detection as a feature. Consider either implementing basic conflict detection (e.g., multiple Python versions, pip vs uv, npm vs yarn) or updating the PR description to note this is a future enhancement.

Would you like me to help implement basic conflict detection logic, such as detecting multiple package managers for the same ecosystem?

tests/core/test_installer.py (1)

31-47: Consider adding a test for unknown platform default.

The tests cover Linux, macOS, and Windows, but per the PR comments, get_platform() defaults to "linux" for unknown platforms. Consider adding a test to verify this behavior:

def test_get_platform_unknown_defaults_to_linux(self):
    """Test that unknown platform defaults to linux."""
    with patch("platform.system", return_value="FreeBSD"):
        assert get_platform() == "linux"

tests/cli/test_env_commands.py (1)

1-23: Consider adding pytestmark = pytest.mark.v2 for CLI tests.

Per coding guidelines, CLI tests should be marked with v2. While the guideline specifically mentions "CLI tests using codeframe.cli.app", these tests exercise env_app which is a sub-application of the main CLI. Consider adding the module-level marker:

 """Tests for environment CLI commands.
 ...
 """
 
+import pytest
 from unittest.mock import patch
 
 from typer.testing import CliRunner
 ...
+
+pytestmark = pytest.mark.v2

As per coding guidelines: "Mark new v2 tests with pytest.mark.v2 decorator or module-level pytestmark = pytest.mark.v2 for CLI tests using codeframe.cli.app"

codeframe/core/installer.py (3)

193-200: Consider catching more specific exceptions.

The bare except Exception is overly broad and may mask unexpected errors. Consider catching specific exceptions like FileNotFoundError or PermissionError separately, or at minimum re-raise KeyboardInterrupt and SystemExit.

♻️ Proposed refinement

-        except Exception as e:
+        except (FileNotFoundError, PermissionError, OSError) as e:
             return InstallResult(
                 tool_name=tool_name,
                 status=InstallStatus.FAILED,
                 message=f"Installation error: {e}",
                 command_used=command,
                 error_output=str(e),
             )

---

564-568: Recording SKIPPED results in history may be misleading.

result.success is True for both SUCCESS and SKIPPED statuses. Recording skipped installations (tool already present) in history with a timestamp could mislead users into thinking an installation occurred. Consider recording only SUCCESS status.

♻️ Suggested change

         # Record in history
-        if result.success:
+        if result.status == InstallStatus.SUCCESS:
             self.record_installation(result)

---

580-586: Silent JSONDecodeError swallows corrupted history without warning.

If the history file is corrupted, the error is silently ignored and an empty history is used. Consider logging a warning so users know their history was lost.

🛡️ Suggested improvement

         if self.history_file.exists():
             try:
                 with open(self.history_file) as f:
                     history = json.load(f)
             except json.JSONDecodeError:
-                pass
+                logger.warning(f"Corrupted history file {self.history_file}, starting fresh")

tests/core/test_installer.py (2)

311-318: Test assertion is broader than the current implementation.

The assertion checks for apt, dnf, or yum, but the SystemInstaller implementation only outputs apt for Linux. This isn't incorrect, but the test may pass if the implementation accidentally changes to a different package manager.

Consider tightening the assertion

     def test_get_install_command_linux(self):
         """Test install command on Linux."""
         installer = SystemInstaller()
         with patch("codeframe.core.installer.get_platform", return_value="linux"):
             cmd = installer.get_install_command("git")
-        assert "apt" in cmd or "dnf" in cmd or "yum" in cmd
+        assert "apt" in cmd  # Current implementation uses apt for Linux
         assert "git" in cmd

---

327-334: Similar assertion broadness for Windows.

The assertion checks for choco or winget, but implementation only uses choco. Consider aligning the test with the actual implementation.

codeframe/core/environment.py (3)

407-423: First-match detection may miss polyglot projects.

Projects with multiple manifest files (e.g., pyproject.toml + package.json) will only be detected as the first-matched type. This is acceptable for v1, but consider documenting this limitation or returning a list of detected types in the future.

---

643-658: _detect_conflicts is a placeholder returning empty list.

The method is documented as detecting tool conflicts but currently returns an empty list. Consider either implementing real conflict detection (e.g., multiple Python versions, conflicting formatters) or adding a TODO comment explaining the intended behavior.

Would you like me to suggest implementation ideas for common tool conflicts (e.g., black vs. yapf formatter conflicts, different Python version requirements)?

---

659-697: Install commands partially duplicate installer.py logic.

The _get_install_command method has hardcoded install commands that overlap with PipInstaller, NpmInstaller, etc. Consider delegating to the installer module to keep commands in sync.

♻️ Suggested approach

+from codeframe.core.installer import ToolInstaller
+
+class EnvironmentValidator:
+    def __init__(self):
+        # ... existing code ...
+        self._tool_installer = ToolInstaller()
+
     def _get_install_command(self, tool_name: str, project_type: str) -> str:
-        # Hardcoded commands...
+        # Delegate to installer for supported tools
+        cmd = self._tool_installer.get_install_command(tool_name)
+        if cmd:
+            return cmd
+        
+        # Fallback for tools not in installer
+        return f"Install {tool_name} using your system package manager"